Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FINNA-2662] Adjust loan history export to use paging #3076

Merged
merged 32 commits into from
Feb 12, 2025

Conversation

LuomaJuha
Copy link

@LuomaJuha LuomaJuha commented Nov 4, 2024

Adjustments:

  • Display max amount of pages to download from, select from which page to start download.
  • Use post
  • Download limits now configurable

TODO:

  • Translations checked

@LuomaJuha LuomaJuha requested review from EreMaijala and removed request for EreMaijala November 4, 2024 14:51
@LuomaJuha
Copy link
Author

Small adjustment to allow more large paging.

Copy link

@EreMaijala EreMaijala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Jos sivutus ei ole käytössä, paginator on false ja downloadhistory.phtml rivi 7 kaatuu
  • Pitäisikö sivujen määrä olla jotenkin suhteessa sivun kokoon? Tyyliin ceil(1000 / sivun pituus)?
  • Lomakkeen toimintaa pitää vielä miettiä. Ehkä olisi parempi, että siinä olisi vaikka checkbox, jolla valitaan, lisätäänkö olemassaolevaan tiedostoon. Nykyinen ei ole kovin ymmärrettävä. Ei se ihan selväksi tulisi sittenkään, kun lopullisen tiedoston tallennuksessa tulee alkuperäinen yliajettavaksi ja siitä tietysti varoitus.
  • Toisaalta en tykkää ajatuksesta, että käyttäjä pystyy lataamaan palvelimelle jonkin sattumanvaraisen tiedoston, jota sitten koitetaan parsia. Tässä on aika merkittävä hyökkäysvektori. Mitä jos tehtäisiin nyt vähän yksinkertaisempana niin, että voi vain tallentaa uuteen tiedostoon tietyt sivut? Jää toki enemmän työtä käyttäjälle, mutta on sekin parempi kuin epäonnistuva vienti.

@EreMaijala
Copy link

@LuomaJuha Perhaps just avoid the modal and display the download options on a separate page?

@LuomaJuha
Copy link
Author

@EreMaijala joo, yksinkertaistan ja poistan tuon tiedostoon jatkamisen. Voi tulla muuten jotain ylimäärästä säätöä tuosta.

@EreMaijala
Copy link

Tuli mieleen jännä ajatus tuosta lainaushistorian lataamisesta: Mitä jos tekisikin sellaisen, että kun ladattavaa on liikaa, näytetään nappula "Lataa osa 1/5". Kun sitä klikkaa, se pistää lataukseen ekan osan ja vaihtaa nappulaksi "Lataa osa 2/5"? Sitten se voisi lisätä tiedoston nimeen myös osan numeron fiksusti, ja lataaminen olisi niin helppoa kuin tällä systeemillä vaan voi.

@LuomaJuha LuomaJuha removed the request for review from EreMaijala November 19, 2024 08:15
@LuomaJuha
Copy link
Author

@EreMaijala Poistan review:n ja laitan uudestaan kun on tuunattu

@LuomaJuha LuomaJuha requested a review from EreMaijala January 15, 2025 17:48
Copy link

@EreMaijala EreMaijala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jotain hämärää tossa on edelleen on noiden sivutusten kanssa. Jos asetuksissa on

[Catalog]
historic_loan_page_size = 3
loan_history_download_batch_limit = 2

Niin jos on 7 lainaa, kertoo oikein, että neljä sivua. Mutta kun laittaa latauksen menemään, niin saa tiedostot:

  1. -1-1, jossa on 3 tietuetta
  2. -2-2, jossa on toiset 3
  3. -3-3, jossa on 1 tietue
  4. -4-4, jossa ei ole mitään

Pitäisikö ton jälkimmäisen numeron tiedoston nimessä olla kokonaismäärä?

@LuomaJuha
Copy link
Author

Hmm, pitääkin katsoa mitäs siel tapahtuu. Toi vika oli ns viimeinen sivu. Veikkaan että jätän sen pois jos on vaan 1 sivu tuloksessa.

@LuomaJuha LuomaJuha requested a review from EreMaijala January 16, 2025 13:41
@EreMaijala
Copy link

@LuomaJuha Tsekkaatko konfliktit? Dropdown-itemit tarttee nyt dropdown-item -luokan.

Copy link

@EreMaijala EreMaijala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tämä toimii nyt nähdäkseni ihan hyvin ja voidaan mergetä, kun konfliktit on tsekattu. Käyttöliittymää voisi ehkä vielä miettiä. Voisi olla selkeämpi, jos lataus avaisi vaikka erillisen sivun, jossa olisi aina lataa seuraava osa -nappula tms.

Suosikkeihin kopioinnin pitäisi tehdä vastaavasti palanen kerrallaan, mutta ilman latausta.

@LuomaJuha
Copy link
Author

@EreMaijala Juu, mie katon! :)

@LuomaJuha LuomaJuha requested a review from EreMaijala February 11, 2025 10:35
@LuomaJuha LuomaJuha requested a review from EreMaijala February 12, 2025 10:14
@EreMaijala EreMaijala merged commit 8059754 into NatLibFi:dev Feb 12, 2025
6 checks passed
@LuomaJuha LuomaJuha deleted the add-paging-to-history branch February 12, 2025 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants